Assert tests with localized values #21982
Conversation
dvdchr
added
[Type] Enhancement
Testing
|
| App Name |  WordPress Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr21982-e0240de | |
| Version | 23.6 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | e0240de | |
| App Center Build | WPiOS - One-Offs #7720 |
|
| App Name |  Jetpack Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr21982-e0240de | |
| Version | 23.6 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | e0240de | |
| App Center Build | jetpack-installable-builds #6736 |
|
Thanks for addressing this! |
|
|
||
| // Then stats are displayed | ||
| XCTAssertEqual(viewModel.impressions, "1,000") | ||
| XCTAssertEqual(viewModel.impressions, 1_000.abbreviatedString()) |
There was a problem hiding this comment.
Thanks for looking into this @dvdchr.
By calling abbreviatedString() here, it seems like we are testing more at the implementation level than the behavior level. That is, reading the test one doesn't learn what the expected impressions value, only that it should be whatever abbreviatedString() returns.
For reference, here's the production code, which, unsurprisingly, calls abbreviatedString():
This is probably okay practically, because we are not likely are we to change this formatting...
Still, I think we would be better served by making the application language and region deterministic, as you suggested in the PR description.
There was a problem hiding this comment.
Still, I think we would be better served by making the application language and region deterministic, as you suggested in the PR description.
Plus 1 on this. While this fixes tests for Indonesian, I tried running DashboardStatsViewModelTests with Arabic as the locale, and it still failed.
|
Hey, a quick update. As per @mokagio and @hassaanelgarem 's inputs, I tried updating the Unfortunately, I'll need to put this on hold this week as I need to wrap up my project work. I'll take another look at my downtime during the meetup. In the meantime, I'll clear the milestone target and set this to Draft for now. |
|
Thank you for the update @dvdchr |
Hi @momo-ozawa and @hassaanelgarem 👋🏼 , I'm assigning y'all as reviewers based on GitHub recommendations.
Some of the unit test cases failed locally because the assertions expected the code to use
,as the thousands separator and.as the decimal separator. However, in my localeen_ID, it is the other way around (e.g.10.000,1,25M).This PR addresses the issue by using the
abbreviatedString(forHeroNumber:)for the expected value so it adjusts properly in different locales.Alternatively, we can also force all local test runs to run in the UTC timezone and use
en_USlocale, but I'm not sure if this is the best way. 🤔 cc-ing @mokagio in case you have thoughts on this.To test
Indonesia.DashboardStatsViewModelTests.testReturnedDataIsFormattedCorrectly()andBlazeCampaignViewModelTests.testViewModel()and verify that they are passing.Regression Notes
Potential unintended areas of impact
Should be none.
What I did to test those areas of impact (or what existing automated tests I relied on)
Ran the tests in both local and US locale.
What automated tests I added (or what prevented me from doing so)
N/A.
PR submission checklist:
RELEASE-NOTES.txtif necessary.UI Changes testing checklist:
Not applicable.